Redact sensitive device authorization debug output - #8269
Conversation
Assisted-By: devx/bb1c6644-dd32-425e-b0c8-33f7efa1bb93
Assisted-By: devx/51bab362-9374-42a8-86d0-152326d57949
There was a problem hiding this comment.
Pull request overview
This PR prevents sensitive OAuth device authorization values (notably device_code and verification_uri_complete) from being emitted in verbose/debug logs, reducing the risk of credential leakage in captured CLI output while keeping the interactive login experience unchanged.
Changes:
- Replaced debug logging of the full device authorization response with a redacted, metadata-only message (HTTP status,
interval,expires_inwith anot providedfallback). - Added regression tests to ensure debug output excludes sensitive fields while preserving expected user-facing info output.
- Added a patch changeset for
@shopify/cli-kitdocumenting the user-visible fix.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/cli-kit/src/private/node/session/device-authorization.ts | Redacts debug output to avoid logging live device authorization credentials. |
| packages/cli-kit/src/private/node/session/device-authorization.test.ts | Adds tests asserting secrets are absent from debug logs and fallback markers render correctly. |
| .changeset/redact-device-authorization-log.md | Patch changeset describing the redaction fix. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
amcaplan
left a comment
amcaplan
left a comment
There was a problem hiding this comment.
Appreciate the change. A couple of comments on making sure our tests add value.
| expect(got).toEqual(dataExpected) | ||
| }) | ||
|
|
||
| test('does not include authorization credentials in debug output', async () => { |
There was a problem hiding this comment.
This feels like a test that "We don't do what we used to do" - but it doesn't actually add a whole lot of value in the current state.
| // Then | ||
| const debugOutput = JSON.stringify(outputDebug.mock.calls) | ||
| expect(debugOutput).toContain('interval=not provided') | ||
| expect(debugOutput).not.toContain('interval=undefined') |
There was a problem hiding this comment.
This last line feels unnecessary
WHY are these changes introduced?
The device authorization debug log serialized the full server response. In verbose mode this wrote
device_code— a live credential while the flow is pending — andverification_uri_completeto the terminal and to anything that captures CLI output.WHAT is this pull request doing?
interval,expires_in. Omitted optional fields render asnot provided.How to test your changes?
Run
shopify auth logout, then any authenticated command with--verbose(for exampleshopify theme list --verbose). Confirm the "Received device authorization response" debug line shows only status/interval/expiry, while the login prompt still shows the user code and link.Checklist
patchfor bug fixes ·minorfor new features ·majorfor breaking changes) and added a changeset withpnpm changeset add